Repository navigation
fix(parts): commit a part before its handoff turn can run (#751) - #772
Merged
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Written-by: Dev
Replaces #771: GitHub closed that one when its head branch was renamed (a mistake on my side) and refuses to reopen it after the force-push, so this is the same branch and the same tip.
Fixes #751
Tip SHA: a882ced
Supersedes #756. Its branch is on the author's fork, and this host cannot push there (a ref probe answers 200, a blob create is 404), so #756 cannot get the regression test review required. This branch carries the author's change —
Co-authored-by: HugoXKis in the commit — plus that test. #756 can be closed once this one is judged.Reproduce
core/test/bot-messages.test.js→ a part is committed before its handoff turn can run, so its failure is rescued once: four turned-down requests move the Bot on twice. The test holds the first part's commit save open —rt.save, the callmoveOnawaits right after it makes the fresh part current — and fails that part's handoff turn while the save is still in flight. That is the window the merge queue kept landing in. On main the failure handler reads a part that is current in memory but not marked rescued, so the second handoff comes out as a third part that is not the rescued one: its note saysThere was no earlier note to carry overinstead of the note of the part before it, and the messages that were supposed to be left out come along.Nothing waits on a timer: the failure is injected inside the save the product itself awaits, and every assertion is read from the recorded handoff.
What changed
core/src/host/parts.js(moveOn): the new segment, the pointer and therescuedmark are set and saved before the handoff package is sent, so the turn the handoff starts cannot see an uncommitted part. The mark goes in before the save awaits — a failure handler that sees this part can never see it unmarked — andrestore()drops pixel and mark together when the save fails. A failed handoff rolls the pointer back, saves the old part best effort and archives the fresh Session, so the stopped turn's input still goes on in the old part.moveSchedulesstays after the package.core/test/bot-messages.test.js: the case above; it asserts the second handoff is part 3, still says the latest messages are left out this time, lists(none), and that only two parts were made. The harness now builds the runtime withcreateRuntimeso a test can hold that one write open.docs/subsystems/core-host.md: the rule, plus the two windows this ordering leaves open — a crash after the save and before the package is flushed (fresh part current, no package: the work goes on there, the note does not, and the rollback never runs), and a failing rollback save (file shows the fresh part, memory the old one).changelog.d/parts-commit-before-handoff.fixed.mdandpatches/ds-bot/(0001,0014) regenerated fromcore/.What I ran
On CI's pair (Node 24.10.0, DSH
0.2.0-rc.2from npm),TZ=UTC:node --import ./test/harness-resolve.mjs --test test/bot-messages.test.js→ 21 tests, 21 pass;test/work-dir.test.js→ 13/13.--test-concurrency=6→ 21/21 every run.--test-concurrency=6: 1254 tests, 1251 pass, 3 fail — the three environment reds this box has on main too (empty diff passes,composer-label,read-page; no git, no jsdom here).patches/ds-bot/files/and the ten patches toFei2411/DeepSeek-Bot@2b10c70reproducescore/byte for byte (only the localnode_modulesof this box is missing).Red without the source change
Two cuts, each restored afterwards (21/21 again after each); the file is green 10 runs at
--test-concurrency=6.core/src/host/parts.jsback to main → the new case is red in 97–99 ms, three runs in a row, onthe rescued part lost the note of the part before it. Actual:[Handoff · part 3] … There was no earlier note to carry over … ## Latest messages (none); expected/so they are left out this time\\. First tell the user/. The other 20 cases stay green (20 pass / 1 fail). The wait that leads there no longer asserts on its own: it settles, thenassert.ok(second, "the second switch was dropped: …")reads as a dropped switch if the handoff never comes, so a red here can never look like a timeout again.rescuedmark back after the save → the same case fails on the same assertion (23 ms), so the mark — not only the order — is what the handler needs.Not run
Risk
T2 (
core/src/host/**). Ordering only, and the failure path now rolls back more state (pointer, segments, questions,rescued) and saves it best effort. A failed handoff still leaves the old part current and hands the stopped turn's input on there, which is whatwork-dir.test.jscovers.